"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +/*
> + * Populate the changed[] arrays from externally supplied hunks,
> + * bypassing the diff algorithm.  Validates that hunks are in order,
> + * non-overlapping, and within bounds.
> + *
> + * Returns 0 on success, -1 on validation failure.
> + */
> +static int xdl_populate_hunks_from_external(xdfenv_t *xe,
> +					    const struct xdl_hunk *hunks,
> +					    size_t nr_hunks)
> +{
> +	size_t i;
> +	long j, prev_old_end = 0, prev_new_end = 0;
> +	long total_old = 0, total_new = 0;
> +
> +	/*
> +	 * Clear changed[] arrays.  xdl_prepare_env() may have dirtied
> +	 * them via xdl_cleanup_records().  The allocation is nrec + 2
> +	 * elements; changed points one past the start (see xprepare.c).
> +	 */
> +	memset(xe->xdf1.changed - 1, 0,
> +	       (xe->xdf1.nrec + 2) * sizeof(bool));
> +	memset(xe->xdf2.changed - 1, 0,
> +	       (xe->xdf2.nrec + 2) * sizeof(bool));

This, especially the starting offset of -1, looks horrible.  The
internal layout of xdfenv_t might happen to match the way the above
code expects, which is how xdl_prepare_ctx() may have give you, but
it somehow feels brittle.  I guess the assumption that changed[]
does not point at the beginning of the allocated area (e.g., it is a
no-no to free(xe->xdf1.changed) or realloc() it) is so pervasive that
it cannot be helped.  Sigh.

>  int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
>  	     xdemitconf_t const *xecfg, xdemitcb_t *ecb) {
>  	xdchange_t *xscr;
>  	xdfenv_t xe;
>  	emit_func_t ef = xecfg->hunk_func ? xdl_call_hunk_func : xdl_emit_diff;
>  
> -	if (xdl_do_diff(mf1, mf2, xpp, &xe) < 0) {
> -
> -		return -1;
> +	if (xpp->external_hunks) {
> +		if (xdl_prepare_env(mf1, mf2, xpp, &xe) < 0)
> +			return -1;
> +		if (xdl_populate_hunks_from_external(&xe,
> +						     xpp->external_hunks,
> +						     xpp->external_hunks_nr) < 0) {
> +			/*
> +			 * Invalid external hunks; fall back to the
> +			 * builtin diff algorithm.  Re-runs
> +			 * xdl_prepare_env() via xdl_do_diff().
> +			 */
> +			xdl_free_env(&xe);
> +			if (xdl_do_diff(mf1, mf2, xpp, &xe) < 0)
> +				return -1;

If the external tool keeps sending bogus hunks, silently falling
back to what we would have done if there weren't any external stuff
may be necessary to pleasantly keep using Git, but two and a half
short comments here.

 (1) "What we would have done" is exactly the same as what appears
     in the corresponding "else" block.  Can we make sure that we do
     not have to keep updating both copies in the future with some
     code rearrangement?

 (2) The writer of the external tool may want to see some trace of
     warning under certain flags when a failure of the tool forces
     the receiving end to fallback.

 (3) If the tool throws too many broken replies, perhaps we want to
     disable it automatically?

> +		}
> +	} else {
> +		if (xdl_do_diff(mf1, mf2, xpp, &xe) < 0)
> +			return -1;
>  	}
> +
>  	if (xdl_change_compact(&xe.xdf1, &xe.xdf2, xpp->flags) < 0 ||
>  	    xdl_change_compact(&xe.xdf2, &xe.xdf1, xpp->flags) < 0 ||
>  	    xdl_build_script(&xe, &xscr) < 0) {

On Thu, May 21, 2026 at 10:29 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +/*
> > + * Populate the changed[] arrays from externally supplied hunks,
> > + * bypassing the diff algorithm.  Validates that hunks are in order,
> > + * non-overlapping, and within bounds.
> > + *
> > + * Returns 0 on success, -1 on validation failure.
> > + */
> > +static int xdl_populate_hunks_from_external(xdfenv_t *xe,
> > +                                         const struct xdl_hunk *hunks,
> > +                                         size_t nr_hunks)
> > +{
> > +     size_t i;
> > +     long j, prev_old_end = 0, prev_new_end = 0;
> > +     long total_old = 0, total_new = 0;
> > +
> > +     /*
> > +      * Clear changed[] arrays.  xdl_prepare_env() may have dirtied
> > +      * them via xdl_cleanup_records().  The allocation is nrec + 2
> > +      * elements; changed points one past the start (see xprepare.c).
> > +      */
> > +     memset(xe->xdf1.changed - 1, 0,
> > +            (xe->xdf1.nrec + 2) * sizeof(bool));
> > +     memset(xe->xdf2.changed - 1, 0,
> > +            (xe->xdf2.nrec + 2) * sizeof(bool));
>
> This, especially the starting offset of -1, looks horrible.  The
> internal layout of xdfenv_t might happen to match the way the above
> code expects, which is how xdl_prepare_ctx() may have give you, but
> it somehow feels brittle.  I guess the assumption that changed[]
> does not point at the beginning of the allocated area (e.g., it is a
> no-no to free(xe->xdf1.changed) or realloc() it) is so pervasive that
> it cannot be helped.  Sigh.
>

Agreed it is ugly. I wanted to make sure the entire changed[] including
sentinels were clear as a defensive measure for downstream callers
(xdl_change_compact). I agree this results in something that is ugly
and brittle, but in the end I thought it was superior to relying on the
fact that upstream zeroes the entire changed[] array. Maybe if the
comment was more explicit about why this is happening it would be
helpful?

    /*
     * Clear changed[] arrays including sentinels.
     * xdl_prepare_env() may have dirtied them via
     * xdl_cleanup_records(), and xdl_change_compact() reads
     * the sentinel at changed[-1] during backward scans.
     */

> >  int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
> >            xdemitconf_t const *xecfg, xdemitcb_t *ecb) {
> >       xdchange_t *xscr;
> >       xdfenv_t xe;
> >       emit_func_t ef = xecfg->hunk_func ? xdl_call_hunk_func : xdl_emit_diff;
> >
> > -     if (xdl_do_diff(mf1, mf2, xpp, &xe) < 0) {
> > -
> > -             return -1;
> > +     if (xpp->external_hunks) {
> > +             if (xdl_prepare_env(mf1, mf2, xpp, &xe) < 0)
> > +                     return -1;
> > +             if (xdl_populate_hunks_from_external(&xe,
> > +                                                  xpp->external_hunks,
> > +                                                  xpp->external_hunks_nr) < 0) {
> > +                     /*
> > +                      * Invalid external hunks; fall back to the
> > +                      * builtin diff algorithm.  Re-runs
> > +                      * xdl_prepare_env() via xdl_do_diff().
> > +                      */
> > +                     xdl_free_env(&xe);
> > +                     if (xdl_do_diff(mf1, mf2, xpp, &xe) < 0)
> > +                             return -1;
>
> If the external tool keeps sending bogus hunks, silently falling
> back to what we would have done if there weren't any external stuff
> may be necessary to pleasantly keep using Git, but two and a half
> short comments here.
>
>  (1) "What we would have done" is exactly the same as what appears
>      in the corresponding "else" block.  Can we make sure that we do
>      not have to keep updating both copies in the future with some
>      code rearrangement?
>

How about something like this:

  if (xpp->external_hunks) {
      if (xdl_prepare_env(mf1, mf2, xpp, &xe) < 0)
          return -1;
      if (xdl_populate_hunks_from_external(&xe,
                       xpp->external_hunks,
                       xpp->external_hunks_nr) == 0)
          goto diff_done;
      xdl_free_env(&xe);
  }

  if (xdl_do_diff(mf1, mf2, xpp, &xe) < 0)
      return -1;

  diff_done:

>  (2) The writer of the external tool may want to see some trace of
>      warning under certain flags when a failure of the tool forces
>      the receiving end to fallback.
>

In diff.c how about we emit a warning rather than a trace on
fallback:

    warning(_("diff process failed for '%s',"
              " falling back to builtin diff"),
            name_a);

>  (3) If the tool throws too many broken replies, perhaps we want to
>      disable it automatically?
>

For the RFC I wanted to keep it simple, but I definitely agree. A configurable
failure policy makes a lot of sense to me (e.g., disable after N failures).

> > +             }
> > +     } else {
> > +             if (xdl_do_diff(mf1, mf2, xpp, &xe) < 0)
> > +                     return -1;
> >       }
> > +
> >       if (xdl_change_compact(&xe.xdf1, &xe.xdf2, xpp->flags) < 0 ||
> >           xdl_change_compact(&xe.xdf2, &xe.xdf1, xpp->flags) < 0 ||
> >           xdl_build_script(&xe, &xscr) < 0) {

Michael Montalbo <mmontalbo@gmail.com> writes:

>> > +      * Clear changed[] arrays.  xdl_prepare_env() may have dirtied
>> > +      * them via xdl_cleanup_records().  The allocation is nrec + 2
>> > +      * elements; changed points one past the start (see xprepare.c).
>> > +      */
>> > +     memset(xe->xdf1.changed - 1, 0,
>> > +            (xe->xdf1.nrec + 2) * sizeof(bool));
>> > +     memset(xe->xdf2.changed - 1, 0,
>> > +            (xe->xdf2.nrec + 2) * sizeof(bool));
>>
>> This, especially the starting offset of -1, looks horrible.  The
>> internal layout of xdfenv_t might happen to match the way the above
>> code expects, which is how xdl_prepare_ctx() may have give you, but
>> it somehow feels brittle.  I guess the assumption that changed[]
>> does not point at the beginning of the allocated area (e.g., it is a
>> no-no to free(xe->xdf1.changed) or realloc() it) is so pervasive that
>> it cannot be helped.  Sigh.
>>
>
> Agreed it is ugly. I wanted to make sure the entire changed[] including
> sentinels were clear as a defensive measure for downstream callers
> (xdl_change_compact). I agree this results in something that is ugly
> and brittle, but in the end I thought it was superior to relying on the
> fact that upstream zeroes the entire changed[] array. Maybe if the
> comment was more explicit about why this is happening it would be
> helpful?

Perhaps make these memset() into calls to a helper function that is
defined in xdiff/xprepare.c with a descriptive name and placed near
where xdl_prepare_ctx() is.  That way, the patch in question does
not even have to expose the strangeness of changed[] (i.e., it has 2
more elements than it would normally contain to make the memory
region for changed[-1] and changed[N] valid, and freeing it requires
free(changed-1)) to the code path.  It only needs to say "Hey, I am
clearing changed[] arrays because of XXX" without having to say "by
the way, the memory layout of changed[] is strange this way", the
latter of which is not exactly of interest for readers of this code.

>     /*
>      * Clear changed[] arrays including sentinels.
>      * xdl_prepare_env() may have dirtied them via
>      * xdl_cleanup_records(), and xdl_change_compact() reads
>      * the sentinel at changed[-1] during backward scans.
>      */

And this belongs in xdiff/xprepare.c near that new helper function.

On Sun, May 24, 2026 at 1:50 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Michael Montalbo <mmontalbo@gmail.com> writes:
>
> >> > +      * Clear changed[] arrays.  xdl_prepare_env() may have dirtied
> >> > +      * them via xdl_cleanup_records().  The allocation is nrec + 2
> >> > +      * elements; changed points one past the start (see xprepare.c).
> >> > +      */
> >> > +     memset(xe->xdf1.changed - 1, 0,
> >> > +            (xe->xdf1.nrec + 2) * sizeof(bool));
> >> > +     memset(xe->xdf2.changed - 1, 0,
> >> > +            (xe->xdf2.nrec + 2) * sizeof(bool));
> >>
> >> This, especially the starting offset of -1, looks horrible.  The
> >> internal layout of xdfenv_t might happen to match the way the above
> >> code expects, which is how xdl_prepare_ctx() may have give you, but
> >> it somehow feels brittle.  I guess the assumption that changed[]
> >> does not point at the beginning of the allocated area (e.g., it is a
> >> no-no to free(xe->xdf1.changed) or realloc() it) is so pervasive that
> >> it cannot be helped.  Sigh.
> >>
> >
> > Agreed it is ugly. I wanted to make sure the entire changed[] including
> > sentinels were clear as a defensive measure for downstream callers
> > (xdl_change_compact). I agree this results in something that is ugly
> > and brittle, but in the end I thought it was superior to relying on the
> > fact that upstream zeroes the entire changed[] array. Maybe if the
> > comment was more explicit about why this is happening it would be
> > helpful?
>
> Perhaps make these memset() into calls to a helper function that is
> defined in xdiff/xprepare.c with a descriptive name and placed near
> where xdl_prepare_ctx() is.  That way, the patch in question does
> not even have to expose the strangeness of changed[] (i.e., it has 2
> more elements than it would normally contain to make the memory
> region for changed[-1] and changed[N] valid, and freeing it requires
> free(changed-1)) to the code path.  It only needs to say "Hey, I am
> clearing changed[] arrays because of XXX" without having to say "by
> the way, the memory layout of changed[] is strange this way", the
> latter of which is not exactly of interest for readers of this code.
>
> >     /*
> >      * Clear changed[] arrays including sentinels.
> >      * xdl_prepare_env() may have dirtied them via
> >      * xdl_cleanup_records(), and xdl_change_compact() reads
> >      * the sentinel at changed[-1] during backward scans.
> >      */
>
> And this belongs in xdiff/xprepare.c near that new helper function.

That sounds a lot nicer. Will update.

"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +struct diff_subprocess {
> +	struct subprocess_entry subprocess;
> +	unsigned int supported_capabilities;
> +};
> +
> +static int subprocess_map_initialized;
> +static struct hashmap subprocess_map;

Can we avoid introducing new global variables like these?  Would
"struct userdiff_driver" or "struct diff_options" be a good place to
hang this hashmap, perhaps?

> +static int send_file_content(int fd, const char *buf, long size)
> +{
> +	int ret;
> +
> +	if (size > 0)
> +		ret = write_packetized_from_buf_no_flush(buf, size, fd);
> +	else
> +		ret = 0;

Shouldn't "size == -24" be flagged as an invalid input?

> +	if (ret)
> +		return ret;
> +	return packet_flush_gently(fd);
> +}

> +static int parse_hunk_line(const char *line, struct xdl_hunk *hunk)
> +{
> +...
> +}

This gives a silent error diagnosis, which is good for a lower level
helper.

> +int diff_process_get_hunks(struct userdiff_driver *drv,
> +			   const char *path,
> +			   const char *old_buf, long old_size,
> +			   const char *new_buf, long new_size,
> +			   struct xdl_hunk **hunks_out,
> +			   size_t *nr_hunks_out)
> +{
> +	struct diff_subprocess *backend;
> +	struct child_process *process;
> +	int fd_in, fd_out;
> +	struct strbuf status = STRBUF_INIT;
> +	struct xdl_hunk *hunks = NULL;
> +	struct xdl_hunk hunk;
> +	size_t nr_hunks = 0, alloc_hunks = 0;
> +	int len;
> +	char *line;
> +
> +	if (!drv || !drv->process)
> +		return -1;

A driver that does not define process is not an error; it is
perfectly normal in the current world order where nobody has such an
external process and even fi this patch lands, external processes
are optional.  So here "return -1" does not mean an error, and
silent return is perfectly fine.

> +	backend = find_or_start_process(drv->process);
> +	if (!backend)
> +		return -1;

This is probably an error; the user specified drv->process, we
either tried to find or start the process and failed.  Isn't it an
event that deserves to be reported in an error message?

> +	if (!(backend->supported_capabilities & CAP_HUNKS))
> +		return -1;

Backend started, but the "hunks" feature is not supported.  Perhaps
in a year or two, this external process protocol may have become so
popular that it gained more capabilities, possibly making get_hunks
obsolete.  We may be looking at such an external process that uses
other capabilities but not this one.  This is not an error, so
silent return is perfectly fine.

> +	process = subprocess_get_child_process(&backend->subprocess);
> +	fd_in = process->in;
> +	fd_out = process->out;
> +
> +	/* Send request */
> +	if (packet_write_fmt_gently(fd_in, "command=hunks\n") ||
> +	    packet_write_fmt_gently(fd_in, "pathname=%s\n", path) ||
> +	    packet_flush_gently(fd_in))
> +		goto error;
> +
> +	/* Send old file content */
> +	if (send_file_content(fd_in, old_buf, old_size))
> +		goto error;
> +
> +	/* Send new file content */
> +	if (send_file_content(fd_in, new_buf, new_size))
> +		goto error;
> +
> +	/* Read hunks until flush packet */
> +	while ((len = packet_read_line_gently(fd_out, NULL, &line)) >= 0 &&
> +	       line) {
> +		if (parse_hunk_line(line, &hunk) < 0)
> +			goto error;
> +		ALLOC_GROW(hunks, nr_hunks + 1, alloc_hunks);
> +		hunks[nr_hunks++] = hunk;
> +	}
> +	if (len < 0)
> +		goto error;
> +
> +	/* Read status */
> +	if (subprocess_read_status(fd_out, &status))
> +		goto error;
> +
> +	if (strcmp(status.buf, "success")) {
> +		if (!strcmp(status.buf, "abort"))
> +			backend->supported_capabilities &= ~CAP_HUNKS;
> +		goto error;
> +	}
> +
> +	*hunks_out = hunks;
> +	*nr_hunks_out = nr_hunks;
> +	strbuf_release(&status);
> +	return 0;
> +
> +error:

All exceptions that lead here look like events that should be
reported to the end-user.

> +	free(hunks);
> +	strbuf_release(&status);
> +	return -1;
> +}

> +/*
> + * Query a diff process for hunks describing the changes
> + * between old_buf and new_buf.
> + *
> + * The backend is a long-running subprocess configured via
> + * diff.<driver>.process.  It receives file content via
> + * pkt-line and returns hunks with 1-based line numbers.
> + *
> + * On success, sets *hunks_out and *nr_hunks_out to a newly allocated
> + * array (caller must free) and returns 0.
> + *
> + * On failure, returns -1.  The caller should fall back to the
> + * builtin diff algorithm.
> + */

I do not agree with this.  If it is a failure, the user should fix
the external process (or disable).  It shouldn't be hidden behind a
fallback.  As I left comments, in this round of implementation,
there are conditions that returns -1 for soemthing that is not an
error (i.e., not configured, or process not supporting the
particular capability) *and* in those cases the caller should fall
back as if nothing happened.  But some error cases, the caller
should't hide them.

On Mon, May 25, 2026 at 6:56 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +struct diff_subprocess {
> > +     struct subprocess_entry subprocess;
> > +     unsigned int supported_capabilities;
> > +};
> > +
> > +static int subprocess_map_initialized;
> > +static struct hashmap subprocess_map;
>
> Can we avoid introducing new global variables like these?  Would
> "struct userdiff_driver" or "struct diff_options" be a good place to
> hang this hashmap, perhaps?
>

Will clean this up.

> > +static int send_file_content(int fd, const char *buf, long size)
> > +{
> > +     int ret;
> > +
> > +     if (size > 0)
> > +             ret = write_packetized_from_buf_no_flush(buf, size, fd);
> > +     else
> > +             ret = 0;
>
> Shouldn't "size == -24" be flagged as an invalid input?
>

Will fix and do a broader audit of input validation and bounds checking.

> > +     if (ret)
> > +             return ret;
> > +     return packet_flush_gently(fd);
> > +}
>
> > +static int parse_hunk_line(const char *line, struct xdl_hunk *hunk)
> > +{
> > +...
> > +}
>
> This gives a silent error diagnosis, which is good for a lower level
> helper.
>
> > +int diff_process_get_hunks(struct userdiff_driver *drv,
> > +                        const char *path,
> > +                        const char *old_buf, long old_size,
> > +                        const char *new_buf, long new_size,
> > +                        struct xdl_hunk **hunks_out,
> > +                        size_t *nr_hunks_out)
> > +{
> > +     struct diff_subprocess *backend;
> > +     struct child_process *process;
> > +     int fd_in, fd_out;
> > +     struct strbuf status = STRBUF_INIT;
> > +     struct xdl_hunk *hunks = NULL;
> > +     struct xdl_hunk hunk;
> > +     size_t nr_hunks = 0, alloc_hunks = 0;
> > +     int len;
> > +     char *line;
> > +
> > +     if (!drv || !drv->process)
> > +             return -1;
>
> A driver that does not define process is not an error; it is
> perfectly normal in the current world order where nobody has such an
> external process and even fi this patch lands, external processes
> are optional.  So here "return -1" does not mean an error, and
> silent return is perfectly fine.
>
> > +     backend = find_or_start_process(drv->process);
> > +     if (!backend)
> > +             return -1;
>
> This is probably an error; the user specified drv->process, we
> either tried to find or start the process and failed.  Isn't it an
> event that deserves to be reported in an error message?
>
> > +     if (!(backend->supported_capabilities & CAP_HUNKS))
> > +             return -1;
>
> Backend started, but the "hunks" feature is not supported.  Perhaps
> in a year or two, this external process protocol may have become so
> popular that it gained more capabilities, possibly making get_hunks
> obsolete.  We may be looking at such an external process that uses
> other capabilities but not this one.  This is not an error, so
> silent return is perfectly fine.
>
> > +     process = subprocess_get_child_process(&backend->subprocess);
> > +     fd_in = process->in;
> > +     fd_out = process->out;
> > +
> > +     /* Send request */
> > +     if (packet_write_fmt_gently(fd_in, "command=hunks\n") ||
> > +         packet_write_fmt_gently(fd_in, "pathname=%s\n", path) ||
> > +         packet_flush_gently(fd_in))
> > +             goto error;
> > +
> > +     /* Send old file content */
> > +     if (send_file_content(fd_in, old_buf, old_size))
> > +             goto error;
> > +
> > +     /* Send new file content */
> > +     if (send_file_content(fd_in, new_buf, new_size))
> > +             goto error;
> > +
> > +     /* Read hunks until flush packet */
> > +     while ((len = packet_read_line_gently(fd_out, NULL, &line)) >= 0 &&
> > +            line) {
> > +             if (parse_hunk_line(line, &hunk) < 0)
> > +                     goto error;
> > +             ALLOC_GROW(hunks, nr_hunks + 1, alloc_hunks);
> > +             hunks[nr_hunks++] = hunk;
> > +     }
> > +     if (len < 0)
> > +             goto error;
> > +
> > +     /* Read status */
> > +     if (subprocess_read_status(fd_out, &status))
> > +             goto error;
> > +
> > +     if (strcmp(status.buf, "success")) {
> > +             if (!strcmp(status.buf, "abort"))
> > +                     backend->supported_capabilities &= ~CAP_HUNKS;
> > +             goto error;
> > +     }
> > +
> > +     *hunks_out = hunks;
> > +     *nr_hunks_out = nr_hunks;
> > +     strbuf_release(&status);
> > +     return 0;
> > +
> > +error:
>
> All exceptions that lead here look like events that should be
> reported to the end-user.
>

Agreed on all points. I will restructure things so errors are flagged when
appropriate (i.e., user specified a process but one was not found / couldn't
start and exceptions) and non-errors are treated as they should be.

> > +     free(hunks);
> > +     strbuf_release(&status);
> > +     return -1;
> > +}
>
> > +/*
> > + * Query a diff process for hunks describing the changes
> > + * between old_buf and new_buf.
> > + *
> > + * The backend is a long-running subprocess configured via
> > + * diff.<driver>.process.  It receives file content via
> > + * pkt-line and returns hunks with 1-based line numbers.
> > + *
> > + * On success, sets *hunks_out and *nr_hunks_out to a newly allocated
> > + * array (caller must free) and returns 0.
> > + *
> > + * On failure, returns -1.  The caller should fall back to the
> > + * builtin diff algorithm.
> > + */
>
> I do not agree with this.  If it is a failure, the user should fix
> the external process (or disable).  It shouldn't be hidden behind a
> fallback.  As I left comments, in this round of implementation,
> there are conditions that returns -1 for soemthing that is not an
> error (i.e., not configured, or process not supporting the
> particular capability) *and* in those cases the caller should fall
> back as if nothing happened.  But some error cases, the caller
> should't hide them.

Will address in a follow-up.

Thank you for the feedback!

"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Zero hunks with status=success means the tool considers the
> files equivalent.  Git skips diff output for that file.

Is "zero hunk" a common word or some random string you invented?  If
the latter, which is I am assuming it to be, you should define what
it means at/before the first use.  Here in the proposed log message,
and ...

>
> Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
> ---
>  Documentation/config/diff.adoc   |   8 +
>  Documentation/gitattributes.adoc |  40 ++++
>  Makefile                         |   1 +
>  diff-process.c                   | 206 +++++++++++++++++++
>  diff-process.h                   |  28 +++
>  diff.c                           |  23 +++
>  t/.gitattributes                 |   1 +
>  t/t4080-diff-process.sh          | 338 +++++++++++++++++++++++++++++++
>  8 files changed, 645 insertions(+)
>  create mode 100644 diff-process.c
>  create mode 100644 diff-process.h
>  create mode 100755 t/t4080-diff-process.sh
>
> diff --git a/Documentation/config/diff.adoc b/Documentation/config/diff.adoc
> index 1135a62a0a..4ab5f60df6 100644
> --- a/Documentation/config/diff.adoc
> +++ b/Documentation/config/diff.adoc
> @@ -218,6 +218,14 @@ endif::git-diff[]
>  	Set this option to `true` to make the diff driver cache the text
>  	conversion outputs.  See linkgit:gitattributes[5] for details.
>  
> +`diff.<driver>.process`::
> +	The command to run as a long-running diff process.
> +	The tool communicates via the pkt-line protocol and returns
> +	hunks that are fed into Git's diff and blame pipelines.
> +	If the tool returns zero hunks, the file is treated as
> +	unchanged for both diff output and blame attribution.
> +	See linkgit:gitattributes[5] for details.

... also here.

I do not know if you mean "the tool returns no hunks" (there is no
"hunk <old_start> <old_count> <new_start> <new_count>" line passed
from the tool over the protocol) or "the tool returns zero-hunk"
(there is a special "zero-hunk" message to signal this particular
condition sent over the protocol), and this description does not
quite help disambiguating between the two.

If the former, then avoid "zero hunks" as it sounds like a noun with
special meaning.  Yes, we can say "tool returns one hunk", "tool
returns 31 hunks", etc., so "tool returns zero hunks" may logically
be correct, but "when the tool returns no hunks with status=success"
is much less confusing, I think.

On Mon, May 25, 2026 at 7:26 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > Zero hunks with status=success means the tool considers the
> > files equivalent.  Git skips diff output for that file.
>
> Is "zero hunk" a common word or some random string you invented?  If
> the latter, which is I am assuming it to be, you should define what
> it means at/before the first use.  Here in the proposed log message,
> and ...
>
> >
> > Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
> > ---
> >  Documentation/config/diff.adoc   |   8 +
> >  Documentation/gitattributes.adoc |  40 ++++
> >  Makefile                         |   1 +
> >  diff-process.c                   | 206 +++++++++++++++++++
> >  diff-process.h                   |  28 +++
> >  diff.c                           |  23 +++
> >  t/.gitattributes                 |   1 +
> >  t/t4080-diff-process.sh          | 338 +++++++++++++++++++++++++++++++
> >  8 files changed, 645 insertions(+)
> >  create mode 100644 diff-process.c
> >  create mode 100644 diff-process.h
> >  create mode 100755 t/t4080-diff-process.sh
> >
> > diff --git a/Documentation/config/diff.adoc b/Documentation/config/diff.adoc
> > index 1135a62a0a..4ab5f60df6 100644
> > --- a/Documentation/config/diff.adoc
> > +++ b/Documentation/config/diff.adoc
> > @@ -218,6 +218,14 @@ endif::git-diff[]
> >       Set this option to `true` to make the diff driver cache the text
> >       conversion outputs.  See linkgit:gitattributes[5] for details.
> >
> > +`diff.<driver>.process`::
> > +     The command to run as a long-running diff process.
> > +     The tool communicates via the pkt-line protocol and returns
> > +     hunks that are fed into Git's diff and blame pipelines.
> > +     If the tool returns zero hunks, the file is treated as
> > +     unchanged for both diff output and blame attribution.
> > +     See linkgit:gitattributes[5] for details.
>
> ... also here.
>
> I do not know if you mean "the tool returns no hunks" (there is no
> "hunk <old_start> <old_count> <new_start> <new_count>" line passed
> from the tool over the protocol) or "the tool returns zero-hunk"
> (there is a special "zero-hunk" message to signal this particular
> condition sent over the protocol), and this description does not
> quite help disambiguating between the two.
>
> If the former, then avoid "zero hunks" as it sounds like a noun with
> special meaning.  Yes, we can say "tool returns one hunk", "tool
> returns 31 hunks", etc., so "tool returns zero hunks" may logically
> be correct, but "when the tool returns no hunks with status=success"
> is much less confusing, I think.

Yes, "zero hunks" was my own invention and I see why it's confusing. Will
update the messaging to use "no hunks" instead and do a broader sweep of
the documentation to clarify the protocol and expected tool behavior.